Skip to content

Fix x509-store for illumos#124856

Merged
bartonjs merged 1 commit intodotnet:mainfrom
gwr:il-x509-store
Feb 27, 2026
Merged

Fix x509-store for illumos#124856
bartonjs merged 1 commit intodotnet:mainfrom
gwr:il-x509-store

Conversation

@gwr
Copy link
Contributor

@gwr gwr commented Feb 25, 2026

Can't use sizof(d->d_name)
Use NAME_MAX or MAXNAMLEN

Without this, the code running on illumos reads only 1 character of the file names in the certificates store and cannot find any certificates.

Copilot AI review requested due to automatic review settings February 25, 2026 13:21
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 25, 2026
@gwr
Copy link
Contributor Author

gwr commented Feb 25, 2026

This is a prerequisite for using System.Net.Security on illumos.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates native X509 store enumeration to avoid relying on sizeof(dirent.d_name) (which is not usable on illumos) when allocating buffers and handling certificate file names.

Changes:

  • Compute the temporary path buffer size using NAME_MAX / MAXNAMLEN (with a fallback) instead of sizeof(d_name).
  • Add a guard to skip .pfx entries whose names won’t fit in the remaining buffer space.
  • Switch filename length computation from strnlen(..., sizeof(d_name)) to strlen(...).

Copilot AI review requested due to automatic review settings February 25, 2026 22:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@bartonjs
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gwr
Copy link
Contributor Author

gwr commented Feb 26, 2026

Can anyone help me interpret the CI build analysis?
Is there anything there I need to fix?

@vcsjones
Copy link
Member

Can anyone help me interpret the CI build analysis?
Is there anything there I need to fix?

The "Build Analysis" is green which is the important one. Outerloop lanes tend to have a number of failures because they are flaky tests. The innerloop build is green, barring some already known failures.

All that to say, "CI looks good for this PR".

@vcsjones vcsjones requested a review from bartonjs February 26, 2026 16:08
Can't use sizof(d->d_name)
Use NAME_MAX instead
@gwr
Copy link
Contributor Author

gwr commented Feb 26, 2026

squashed.

@gwr
Copy link
Contributor Author

gwr commented Feb 26, 2026

The failing check seems to have nothing to do with this change.

@bartonjs bartonjs merged commit 806904a into dotnet:main Feb 27, 2026
104 of 106 checks passed
@bartonjs
Copy link
Member

bartonjs commented Feb 27, 2026

The failing check seems to have nothing to do with this change.

Yeah, that's what the Build Analysis conclusion was, too.

@bartonjs
Copy link
Member

squashed.

We do squash-merges, so it isn't necessary to do a squash before the merge. It actually just slowed things down, since I had to wait for the CI legs to all finish again. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Security community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants